Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Lensflare.js to be able to dispose textures #13355

Merged
merged 2 commits into from
Feb 19, 2018
Merged

Conversation

gadlol
Copy link
Contributor

@gadlol gadlol commented Feb 17, 2018

Update Lensflare.js so to be able to properly dispose the textures otherwise we have memory issues...

In order to properly dispose the textures added into the Lensflare ( new THREE.Lensflare() ) by the ( new THREE.LensflareElement() ) we must place them into the object. So then we can do for example:

for( var i = scene.children.length - 1; i >= 0; i-- ) {
  for( var j = 0, n = scene.children[i].elements.length; j <n; j++ ){
    scene.children[i].elements[j].texture.dispose();
  }
}

screenshot from 2018-02-17 23 03 35

Note: we could probably use the "children" array to store the THREE.LensflareElement instead of "elements".

Note: the Lensflare dispose method does not dispose textures.

Update Lensflare.js so to be able to properly dispose the textures otherwise we have memory issues...

In order to properly dispose the textures added into the Lensflare ( new THREE.Lensflare()  ) by the ( new THREE.LensflareElement() ) we must place them into the object. So then we can do for example:

for( var i = scene.children.length - 1; i >= 0; i-- ) {
  for( var j = 0, n = scene.children[i].elements.length; j <n; j++ ){
    scene.children[i].elements[m].texture.dispose();
  }
}
@gadlol gadlol changed the title Update Lensflare.js to properly dispose textures Update Lensflare.js to be able to dispose textures Feb 17, 2018
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 18, 2018

I would keep elements private and just set elements = []; in Lensflare.dispose() . It's the task of the user to keep track of the lens flare textures and dispose them, if necessary.

@gadlol
Copy link
Contributor Author

gadlol commented Feb 18, 2018

By setting elements = []; the textures do not get removed from WebGLRenderer memory. That is a 100% sure, you can check it in the webGLRenderer.info.memory.textures .

So in the Lensflare.dispose() we could make a loop and use elements[i].texture.dispose()

BUT Actually its not only about disposing textures.

What if you want to change on the fly for example the size of the LensflareElement ?

By having elements private you cannot access LensflareElement properties.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 18, 2018

By setting elements = []; the textures do not get removed from WebGLRenderer memory.

Yeah, that's clear. I refer to something different. If the user creates textures, then it's also his responsibility to dispose them. Why? Because, the user can apply these textures also to other materials. Lensflare won't know about this.

Lensflare.dispose() should focus on entities, that are actually allocated by Lensflare. Setting elements = []; should just free references.

@mrdoob
Copy link
Owner

mrdoob commented Feb 18, 2018

I think this is modification to Lensflare's own dispose function that @Mugen87 is proposing:

this.dispose = function () {

	material1a.dispose();
	material1b.dispose();
	material2.dispose();

	tempMap.dispose();
	occlusionMap.dispose();

	for ( var i = 0; i < elements.length; i ++ ) {

		elements[ i ].texture.dispose();

	}

};

And I agree 👍

@mrdoob mrdoob added this to the r91 milestone Feb 18, 2018
@gadlol
Copy link
Contributor Author

gadlol commented Feb 18, 2018

@mrdoob better use : for ( var i = 0, l = elements.length; i < l; i ++ ) { just like in the line 223

@mrdoob
Copy link
Owner

mrdoob commented Feb 18, 2018

Indeed!

Would you like to amend the PR?

@gadlol
Copy link
Contributor Author

gadlol commented Feb 18, 2018

Would you like to amend the PR?

I am sorry but I don't understand what you mean by that...

@mrdoob
Copy link
Owner

mrdoob commented Feb 18, 2018

Would you like to change this PR with the proposed implementation?

@gadlol
Copy link
Contributor Author

gadlol commented Feb 18, 2018

yes

@@ -127,7 +127,7 @@ THREE.Lensflare = function () {

//

var elements = [];
elements = [];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to keep the var here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes i did not notice that...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll clean it up after merging.

@mrdoob mrdoob merged commit 33b42ef into mrdoob:dev Feb 19, 2018
@mrdoob
Copy link
Owner

mrdoob commented Feb 19, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants